Skip to content

Do not require AZURE_USERNAME for shared cache#8095

Merged
ellismg merged 4 commits intoAzure:masterfrom
ellismg:ellismg/fix-7944
Oct 24, 2019
Merged

Do not require AZURE_USERNAME for shared cache#8095
ellismg merged 4 commits intoAzure:masterfrom
ellismg:ellismg/fix-7944

Conversation

@ellismg
Copy link
Copy Markdown
Member

@ellismg ellismg commented Oct 21, 2019

Previously, a username was required when using the
SharedTokenCacheCredential, in order to handle the case where multiple
identities were found in the cache. Since it is common to have only a
single account in your user cache (e.g. you have signed in with only a
single identity), we should allow reading from the cache even when an
explicit AZURE_USERNAME is not specified, if there is exactly one
account in the cache.

When username is unset, if we can not find a token in the cache or we
find multiple tokens, a ClientAuthenticationError error is raised,
with the text "No cached token found". This is similar to how other
cache related failures are handled by the API (they raise this error
with similar text but it includes a hint about what username was used.)

As part of this work, DefaultAzureCredential now unconditionally uses
the shared cache on supported platforms.

This behavior matches how we handle this case in both the .NET and Java
SDKs.

Fixes #7944

@ellismg ellismg requested a review from chlowell October 21, 2019 22:07
@ellismg ellismg requested a review from schaabs as a code owner October 21, 2019 22:07
@adxsdk6
Copy link
Copy Markdown

adxsdk6 commented Oct 21, 2019

Can one of the admins verify this patch?

Comment thread sdk/identity/azure-identity/azure/identity/_authn_client.py Outdated
Comment thread sdk/identity/azure-identity/azure/identity/_authn_client.py Outdated
Comment thread sdk/identity/azure-identity/azure/identity/_authn_client.py Outdated
Comment thread sdk/identity/azure-identity/azure/identity/aio/_authn_client.py Outdated
Comment thread sdk/identity/azure-identity/azure/identity/aio/_authn_client.py Outdated
Comment thread sdk/identity/azure-identity/azure/identity/_credentials/user.py Outdated
@mayurid mayurid assigned ellismg and BernieEllis and unassigned BernieEllis Oct 23, 2019
@mayurid mayurid added Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. labels Oct 23, 2019
@ellismg
Copy link
Copy Markdown
Member Author

ellismg commented Oct 24, 2019

Okay @chlowell and @schaabs, based on our in person discussions, I have updated the code. Once you have signed off, I'll contact shiproom.

ellismg and others added 4 commits October 24, 2019 10:12
Previously, a username was required when using the
SharedTokenCacheCredential, in order to handle the case where multiple
identities were found in the cache. Since it is common to have only a
single account in your user cache (e.g. you have signed in with only a
single identity), we should allow reading from the cache even when an
explicit AZURE_USERNAME is not specified, if there is exactly one
account in the cache.

When username is unset, if we can not find a token in the cache or we
find multiple tokens, a `ClientAuthenticationError` error is raised,
with the text "No cached token found".  This is similar to how other
cache related failures are handled by the API (they raise this error
with similar text but it includes a hint about what username was used.)

As part of this work, `DefaultAzureCredential` now unconditionally uses
the shared cache on supported platforms.

This behavior matches how we handle this case in both the .NET and Java
SDKs.

Fixes #7944
Copy link
Copy Markdown
Member

@chlowell chlowell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for making it happen 🎂

Comment thread sdk/identity/azure-identity/azure/identity/aio/_authn_client.py
@joshfree joshfree requested a review from jianghaolu October 24, 2019 17:49
@ellismg ellismg merged commit de802a5 into Azure:master Oct 24, 2019
@ellismg ellismg deleted the ellismg/fix-7944 branch October 24, 2019 18:16
fengzhou-msft pushed a commit that referenced this pull request Nov 5, 2019
Previously, a username was required when using the
SharedTokenCacheCredential, in order to handle the case where multiple
identities were found in the cache. Since it is common to have only a
single account in your user cache (e.g. you have signed in with only a
single identity), we should allow reading from the cache even when an
explicit AZURE_USERNAME is not specified, if there is exactly one
account in the cache.

When username is unset, if we can not find a token in the cache or we
find multiple tokens, a `ClientAuthenticationError` error is raised,
with the text "No cached token found".  This is similar to how other
cache related failures are handled by the API (they raise this error
with similar text but it includes a hint about what username was used.)

As part of this work, `DefaultAzureCredential` now unconditionally uses
the shared cache on supported platforms.

This behavior matches how we handle this case in both the .NET and Java
SDKs.

Fixes #7944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should we requrie AZURE_USERNAME being set to use the shared cache from DefaultAzureCredential?

8 participants